Skip to content

libnvme/cmds: change copy desc format 0h and 2h elbt to big-endian#3183

Open
ikegami-t wants to merge 9 commits intolinux-nvme:masterfrom
ikegami-t:copy-elbt-endian
Open

libnvme/cmds: change copy desc format 0h and 2h elbt to big-endian#3183
ikegami-t wants to merge 9 commits intolinux-nvme:masterfrom
ikegami-t:copy-elbt-endian

Conversation

@ikegami-t
Copy link
Contributor

@ikegami-t ikegami-t commented Mar 13, 2026

Since the format 1h and 3h elbt values are set as big-endian. Also change the field name eilbrt to elbt as following spec.

@ikegami-t
Copy link
Contributor Author

ikegami-t commented Mar 13, 2026 •
edited
Loading

@igaw @keithbusch @brandon-paupore-sndk @zwxdg @chenghong085 Could you please review the changes? For the issue: #2975 I have rejected the PR: #2977 then the issue also closed. To make consistent just created this PR changes but for the changes I considered again so still the issue: #2975 mentioned seems correct also.. To make sure should we ask to NVMe org about this specification?
Note: I am thinking to change elbat and elbatm fileds also to big-endian to make consistent (but not sire sure if correct or not).

@igaw
Copy link
Collaborator

igaw commented Mar 13, 2026

Would it be possible to have a unit tests for this? Given the amount of discussion around this API I think it would be great to have one. Maybe we could get a binary copy of an existing log page and decode it then?

@ikegami-t
Copy link
Contributor Author

ikegami-t commented Mar 13, 2026

Okay will do try it later. Thank you.

@ikegami-t
Copy link
Contributor Author

ikegami-t commented Mar 14, 2026

Just created the separated PR: #3185 for the unit test.
Note: The test expected data is depended on the current API: nvme_init_copy_range if the API behavior changed then the data also needed to be changed.

igaw
igaw reviewed Mar 16, 2026
View reviewed changes
libnvme/src/nvme/types.h Outdated
Copy link
Collaborator

@igaw igaw Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should rename this struct to nvme_copy_range_f0 because also the spec names this f0.

@igaw
Copy link
Collaborator

igaw commented Mar 16, 2026 •
edited
Loading

Looks good. I saw that there are a handful of new fields defined in the newest spec. We might want to add them too. WDYT?

BTW, the commit message doesn't match anymore. This change updates the names of the fields.

@ikegami-t
Copy link
Contributor Author

ikegami-t commented Mar 16, 2026

Yes I will do rename the struct and add new fields by additional commits. Thank you.

@ikegami-t ikegami-t force-pushed the copy-elbt-endian branch 2 times, most recently from 694c106 to cf45db5 Compare March 17, 2026 15:09
@igaw
Copy link
Collaborator

igaw commented Mar 18, 2026

Sorry for my recent changes and you had to rebase your work. I'll just have to read up on the little to big endian change in your explanation. It strikes me a bit odd that one field is big endian, but that might just be my OCD...

@ikegami-t
Copy link
Contributor Author

ikegami-t commented Mar 18, 2026 •
edited
Loading

No problem. The changes to move the nvme_init_* implementation to header look good. Just changed the odd big endian field to array as same with the copy range format 1 and 3.
Note: The x390x s930x cross build error will be fixed later.

@ikegami-t
libnvme/cmds: change copy desc format 0h and 2h elbt to big-endian
06b7c37 
Since the format 1h and 3h elbt values are set as big-endian.
Also change the field name eilbrt to elbt as following spec.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/test: change copy desc format 0h expected data
1bacd4c 
Since the elbt field changed to big-endian.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme: change copy_range name to copy_range_f0
4d456e9 
Change both the struct name and the init API names.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/types: add nvme_copy_range_f0 new feilds
a90cb15 
Just followed NVMe command set spec revision 1.2.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/types: add nvme_copy_range_f2 new feilds
dc5c054 
Just followed NVMe command set spec revision 1.2.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/types: add nvme_copy_range_f1/f3 new feilds
f5f526c 
Just followed NVMe command set spec revision 1.2.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/types: change copy desc format 0h and 2h elbt to array
4fe3b3e 
Change odd big-endian field as same with the format 1h and 2h.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/cmds: change copy desc format elbatm and elbat to big-endian
612a752 
Since the format elbt values are set as big-endian then follow it.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
libnvme/cmds: fix nvme_init_copy_range_elbt for big endian build
b1898b7 
Since the function just depended on the little endian build.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
Copy link
Contributor Author

ikegami-t commented Mar 21, 2026

Just changed the copy desc format elbat and elabatm fields as big endian and refixed for the s930x crosss build error.
Note: Still not usre if the copy desc format elbt fields are really big endian as not described clearly and the specification is desribed as below as Unless otherwise specified, this specification specifies data in a little endian format..
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@igaw igaw igaw left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ikegami-t @igaw